Skip to content

fix(datepicker): collapse month/year grid after selection#39

Open
matharoo wants to merge 2 commits into
digitalspace:masterfrom
matharoo:fix/datepicker-month-year-view-toggle
Open

fix(datepicker): collapse month/year grid after selection#39
matharoo wants to merge 2 commits into
digitalspace:masterfrom
matharoo:fix/datepicker-month-year-view-toggle

Conversation

@matharoo

Copy link
Copy Markdown

NgdsCalendar drove its *ngIf views off the @input() displayDepth, which round-trips through NgdsCalendarManager — leaving the date grid and the month/year grid visible at once. Track view depth in a local updated directly in the calendar's own click handlers so its *ngIf re-evaluates immediately; still emit changeDepth/Month/Year for nav.

Also widen NgdsDropdown.dropdownClasses to string | string[].

Fixes the issue: bcgov/reserve-rec-public#569

  NgdsCalendar drove its *ngIf views off the @input() displayDepth, which
  round-trips through NgdsCalendarManager — leaving the date grid and the
  month/year grid visible at once. Track view depth in a local
  updated directly in the calendar's own click handlers so its *ngIf
  re-evaluates immediately; still emit changeDepth/Month/Year for nav.

  Also widen NgdsDropdown.dropdownClasses to string | string[].
@marklise

Copy link
Copy Markdown
Member

Heads up — this looks solid for single datepickers, but it regresses the two-calendar range picker.

The single-datepicker fix is correct: owning view depth locally so the *ngIf collapses synchronously is exactly right, and it resolves #569. The dropdownClasses: string | string[] widening is fine too (it's only consumed via [ngClass], which accepts both).

The problem is in dateRange mode. NgdsCalendar now owns depth locally and per-instance, but NgdsCalendarManager still keeps its own displayDepth and assumes both calendars change depth together. That assumption no longer holds, so the two get into a split-brain state. Repro on the Inline rangepicker demo ([dateRange]="true", default minMode=0):

  1. Toggling month/year on one calendar desyncs the pair. Start: left = June 2026 (days), right = July 2026 (days). Click the "June" header on the left calendar → left correctly switches to month view, but the right calendar stays in the day grid and jumps forward a full year to June 2027. You end up with one calendar in month view (2026) and the other in date view (2027). Cause: calendarManager.toggleDepth(1) still advances endCalendar by a year on the assumption it'll also flip to month view — which it no longer does.

  2. The arrows then step by the wrong unit. After that toggle, manager.displayDepth is stuck at 1, so changeDisplayByValue is in "month mode." Clicking the right calendar's next arrow — while it's showing a day grid — moves it a whole year (June 2027 → June 2028) instead of one month.

Root cause is the dual source of truth: child-local depth vs. manager-level displayDepth. The manager still uses displayDepth to (a) offset the end calendar in toggleDepth and (b) decide the arrow step unit in changeDisplayByValue, but it's no longer the thing actually driving what each calendar renders.

Also minor: @Input() displayDepth on NgdsCalendar is now dead (still bound in calendar-manager.component.html but unused in the template/TS) — worth removing to avoid confusion about which depth is authoritative.

Single-calendar configs (dateRange=false, and hideSecondCalendar=true) are unaffected. Direction needs to keep the two calendars' depth in sync again while preserving the synchronous local update.

@danieltruong

Copy link
Copy Markdown
Contributor

calendar.component.ts:

  • risk (L127): The depth state is now local. In dateRange mode, NgdsCalendarManager renders two calendars. Calling toggleDepth (L137) on one instance will not update the other, leading to a visual desync where one calendar shows months/years while the other shows dates. Consider lifting state back to NgdsCalendarManager or using a shared Subject.
  • nit (L24): @input() displayDepth is no longer used in the component logic. Remove to avoid confusion.
  • nit (L133): this.depth is initialized to this.minDisplayDepth in ngOnInit. If the parent CalendarManager starts with a higher displayDepth, the component will be out of sync upon initialization.

calendar.component.html:

  • risk (L48, L72, L83): *ngIf conditions now rely on local depth, confirming the desync behavior in dual-calendar mode.

ngds-dropdown.component.ts:

  • nit (L28): Widening dropdownClasses to string | string[] correctly supports standard [ngClass] usage.

@matharoo

Copy link
Copy Markdown
Author

i have pushed another update, addressing the concerns above:

previously each calendar owned its view depth locally, while the manager kept its own displayDepth and assumed both calendars moved together, so toggling one didn't move the other, and the manager's depth no longer matched what was on screen.

Fix lifts the depth back to NgdsCalendarManager as the single source of truth. It now pushes the depth into both calendars synchronously whenever it changes (via @ViewChildren → a setDepth() on the calendar), so the two stay in lockstep. The per-calendar local write in the click handlers stays, so the clicked calendar still collapses its grid synchronously, the push just brings the other one along in the same tick. Because both calendars actually follow the depth again, the end-calendar offset in toggleDepth and the arrow step unit in changeDisplayByValue are correct without any changes to that maths.

Specifics:

  • Removed the now-unused @input() displayDepth from NgdsCalendar and its template bindings.
  • Both calendars are seeded from the manager's depth in ngAfterViewInit, so they're in sync on init even if the starting depth isn't the minimum.
  • Single-datepicker and hideSecondCalendar paths are unchanged; dropdownClasses left as string | string[].

I tested it by running the ng serve in ngds-toolkit/projects/ngds-forms and checking inline rangepicker : toggling month/year on either calendar now switches both together, and the arrows step by the right unit afterward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants